Skip to content

Conversation

@lcian
Copy link
Member

@lcian lcian commented Dec 17, 2025

This starts the work on implementing Batch Operations by introducing a batch endpoint and an extractor to validate and parse our batch request format.

The implementation roughly follows the spec linked above, tomorrow I will make sure to sync up the two.

@lcian lcian changed the title wip feat: Batch request endpoint and extractor Dec 17, 2025
@lcian lcian marked this pull request as ready for review December 17, 2025 10:00
@lcian lcian requested a review from a team as a code owner December 17, 2025 10:00
@lcian lcian changed the title feat: Batch request endpoint and extractor feat: Batch endpoint and extractor Dec 19, 2025
@lcian lcian force-pushed the lcian/feat/batching-server branch from 557301e to e01966b Compare December 19, 2025 13:07
@lcian lcian changed the title feat: Batch endpoint and extractor feat(server): Add batch endpoint and extractor Dec 19, 2025
@lcian lcian requested a review from matt-codecov January 7, 2026 17:01
}

pub struct BatchRequest {
pub operations: BoxStream<'static, anyhow::Result<Operation>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you replace this anyhow::Result<Operation> with Result<Operation, BatchError> where BatchError (or whatever it should be named) is a new type that derives thiserror::Error? example from auth https://github.com/getsentry/objectstore/blob/main/objectstore-server/src/auth/error.rs

anyhow doesn't really let us differentiate between an error that should return a 400 BAD REQUEST and an error that should return a 500. i'm not sure we should be using it at all, or at least we should push our usage of it as high as we can imo

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is something I noticed as well and that's why I wanted to move to using structured errors with thiserror instead of using anyhow https://linear.app/getsentry/issue/FS-220/improve-error-handling
It might be convenient for me to do that first, and then rebase this PR on top.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think the two have to be coupled. i think you can just return Result<Operation, BatchError> and the error can be converted to anyhow::Error which can be converted to AnyhowResponse in the endpoint implementation. then this file can be left alone when doing FS-220. but up to you, it'll be done at some point either way

where
S: Send + Sync,
{
type Rejection = Response;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FromRequest requires that your Rejection impl IntoResponse, i think you can use (StatusCode, &'static str) as your Rejection and skip calling .into_response() every time?

merni = { workspace = true }
mimalloc = { workspace = true }
mime = "0.3.17"
multer = "3.1.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@lcian lcian Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I think we can use axum::extract::Multipart directly now.
With the old design, which included a manifest of operations as the first part, the correct content-type would've been multipart/mixed and therefore I had to use multer (which axum::extract::Multipart uses under the hood) directly because axum::extract::Multipart only works with multipart/form-data 😅.
Now multipart/form-data is correct, so we can move to using axum::extract::Multipart directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants